Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Crash on iOS devices with half image cropping #1582

Closed
wants to merge 3 commits into from

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented Apr 15, 2022

On iOS, the native code assumes coordinates start at 0,0.
The only crop we can provide is based on the size.

This PR splits CameraImageCropper into two implementations:

  • A default one (what was previously implemented)
  • An iOS one, where the crop is only based on the size

Will fix #1581

…ith a custom size.

Coordinates are also hardcoded
@g123k g123k added 🍎 iOS iOS specific issues or PRs camera labels Apr 15, 2022
@g123k g123k requested a review from a team as a code owner April 15, 2022 07:37
@g123k g123k self-assigned this Apr 15, 2022
@github-actions github-actions bot added the 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… label Apr 15, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2022

Codecov Report

Merging #1582 (d4e3603) into develop (2ea0da3) will decrease coverage by 0.25%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           develop   #1582      +/-   ##
==========================================
- Coverage     8.86%   8.61%   -0.26%     
==========================================
  Files          161     161              
  Lines         6623    6816     +193     
==========================================
  Hits           587     587              
- Misses        6036    6229     +193     
Impacted Files Coverage Δ
...mooth_app/lib/pages/scan/camera_image_cropper.dart 0.00% <0.00%> (ø)
...ges/smooth_app/lib/pages/product/summary_card.dart 0.00% <0.00%> (ø)
...mooth_app/lib/generic_lib/widgets/smooth_card.dart 0.00% <0.00%> (ø)
...oth_app/lib/pages/onboarding/country_selector.dart 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ea0da3...d4e3603. Read the comment docs.

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @g123k!
I have minor remarks about the way you handle abstraction, but I could survive without fixes.

My main concern is that the whole point of those classes is to get better performance by copying less data, e.g. half less data for the top half.

What I understand in your iOS code is that

  • you still copy the whole thing (therefore no expected performance gain)
  • while pretending the width and the height are smaller than the full width and height (= what you copied), which may cause incoherences between what you copied and getSize.
  • and for that you copy/paste existing code from CameraImageFullGetter

If your intention is for the iOS not to crash, you can simply call CameraImageFullGetter just in the iOS case. But copying the whole data while pretending it's less data sounds confusing and a source of errors.

Am I right?

Comment on lines 223 to 242
late int _width;
late int _height;

void _computeCropParameters() {
if (orientation == 0) {
_width = _computeWidth();
_height = _computeHeight();
} else if (orientation == 90) {
_width = _computeWidth();
_height = _computeHeight();
} else {
throw Exception('Orientation $orientation not dealt with for the moment');
}
}

@override
Size getSize() => Size(
_width.toDouble(),
_height.toDouble(),
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All that could be in the super class CameraImageCropper.

_height.toDouble(),
);

// Same implementation as [CameraImageFullGetter]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit surprised: what's the added value if you copy the whole set of planes?
We would be better off with CameraImageFullGetter, wouldn't we?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically it's a CameraImageFullGetter with a custom size.
Since this class still an "ImageCropper", using a mixin may be a solution

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically it's a CameraImageFullGetter with a custom size.

That's what I read, but I have serious doubts about copying a full size while saying it's half size. It's not worth the complication anyway as there's no performance gain.

Since this class still an "ImageCropper", using a mixin may be a solution

I'm not familiar with mixin, but from what I read that's too complex for what we want. Assuming your idea of "full copy but half getSize" is correct, you could just extend CameraImageFullGetter and override getSize.
In the end we don't need a CameraImageCropper anyway, we need an AbstractCameraImageGetter.

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not in favor of code being copied/pasted, and I suggest that _CameraImageCropperImplIOS should extend CameraImageFullGetter.

import 'package:flutter/material.dart';
import 'package:google_ml_barcode_scanner/google_ml_barcode_scanner.dart';
import 'package:smooth_app/pages/scan/abstract_camera_image_getter.dart';
import 'package:typed_data/typed_buffers.dart';

/// Camera Image Cropper, in order to limit the barcode scan computations.
///
/// Note: on iOS, we can only crop the size, as the coordinates are hardcoded.
/// [width01] and [height01] are thus ignored on this platform.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean "[left01], [top01]"?

@teolemon
Copy link
Member

teolemon commented May 9, 2022

Ok, so there are 2 things in this PR: focus and cropping.
We need focus in develop ASAP. We can see cropping later.
WDYT @monsieurtanuki @g123k ?

@monsieurtanuki
Copy link
Contributor

@teolemon @g123k Let's focus on focus: a new PR based only on focus, that uses CameraImageFullGetter (= no crop)

@g123k
Copy link
Collaborator Author

g123k commented May 9, 2022

Ok, will do that!
I change this PR to a draft state

@g123k g123k marked this pull request as draft May 9, 2022 13:34
@g123k g123k removed this from the V1 milestone May 9, 2022
@teolemon teolemon added this to the V1 milestone May 17, 2022
@g123k
Copy link
Collaborator Author

g123k commented May 18, 2022

Now that we have a fork of the library, I will work on the iOS code directly to support the cropping feature

@g123k g123k closed this May 18, 2022
@M123-dev M123-dev deleted the fix_ios_half_image_crash branch May 26, 2022 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
camera 🍎 iOS iOS specific issues or PRs 🎯 P1 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion…
Development

Successfully merging this pull request may close these issues.

[Scan] Half-image decoding is crashing on iOS
4 participants